Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/issue#1692 #1778

Merged
merged 21 commits into from
Apr 3, 2024
Merged

Fix/issue#1692 #1778

merged 21 commits into from
Apr 3, 2024

Conversation

zg009
Copy link
Contributor

@zg009 zg009 commented Mar 21, 2024

See here: #1692

Added a check in put.js handler to see if a client is attempting to create an invalid resource uri

Alain, before this gets merged, let me know if there is a better way to get the RESERVED_SUFFIXES, and if the function should be moved to ldp.js instead

@zg009 zg009 requested review from bourgeoa and TallTed March 21, 2024 17:54
Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

human-facing language tweaks for clarity

lib/handlers/put.js Outdated Show resolved Hide resolved
lib/handlers/put.js Outdated Show resolved Hide resolved
lib/handlers/put.js Outdated Show resolved Hide resolved
zg009 and others added 3 commits March 21, 2024 13:53
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
@bourgeoa
Copy link
Member

bourgeoa commented Mar 21, 2024

@zg009 Thanks for looking at this issue.

  • Both PUT and PATCH are allowed to create intermediate Container
  • PUT can create Container, may fail
  • POST can create Container with slug, never fails and return location

The container name

  1. cannot be the same as an existing resource name
  2. cannot end with a reserved auxiliary extension, to not forbid creation of an auxiliary resource

async checkItemName (url) {
is where point 1. is checked.

Point 2. may be added in this above function

@bourgeoa
Copy link
Member

@zg009
Not sure of POST creating a container with slug. I don't think sure such a test exist

if (this.isAuxResource(slug, extension)) throw error(403, 'POST is not allowed for auxiliary resources')

Could you add a test for POST trying to create a Container with an unallowed slug

@zg009
Copy link
Contributor Author

zg009 commented Mar 22, 2024

There are tests for invalid slug on POST requests here: https://github.com/nodeSolidServer/node-solid-server/blob/f5652f3dfa3ae630408125af99bcf90b7fe21c0d/test/integration/http-test.js#L863C4-L876C7

I added the check I believe you requested

@bourgeoa
Copy link
Member

Sorry I am on a tablet. Not easy to code

  • You are testing a POST creating a resource. I don't see why it should fail.
    POST never fail. The name of the resource created must be checked on the location header.
    Solid specification introduced an exception 409 for auxiliary resources because you cannot be unsure on the resource name in auxiliary resources

  • You are not testing a POST creating a container with a slug ending with .acl
    Below an exemple. Notice the link that say I want a container. This is explicitly needed because a slug cannot end with /

    it('should create container with new slug as a resource', function (done) {

The test should create a container. The container name can be found in location header see next test.

Your code in ldp.js should do something like

If (container) {
  extension = ''
 If slug ends with unallowed extension remove unallowed extension from slug
}
elseif (slug) test slug to not allow creation of auxiliary --> 409

@zg009
Copy link
Contributor Author

zg009 commented Mar 23, 2024

Alain, I do not see how you can do what you are stating is possible.

If slug has extension and extension is .acl or .meta, 403 is thrown
server.post('/post-resources/').set('slug', 'post-resource.acl') // throws 403
server.post('/post-resources/').set('slug', 'post-resource.meta') // throws 403

If slug has no extension, it becomes container.
server.post('/post-resources/').set('slug', 'inner-container') // creates container /post-resources/inner-container/

I do not know how the situation you are describing can exist.

@bourgeoa
Copy link
Member

bourgeoa commented Mar 23, 2024

@zg009 Could you review at my changes
Basically

  • the issue is only for container creation
  • the check is only at the extension part (endsWith)

Can you

  • add a test for .meta
  • TODO find a better test name on line 894

@zg009
Copy link
Contributor Author

zg009 commented Mar 25, 2024

@bourgeoa I renamed it and added some clarity on the new .meta and .acl tests

lib/ldp.js Outdated Show resolved Hide resolved
Copy link
Member

@bourgeoa bourgeoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some console in lib/ldp.js to remove

lib/ldp.js Outdated Show resolved Hide resolved
@bourgeoa
Copy link
Member

Thanks LGTM just remove the 2 console.log() in the recursive

Copy link
Member

@bourgeoa bourgeoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zg009 zg009 requested a review from TallTed March 30, 2024 01:09
@bourgeoa bourgeoa linked an issue Mar 31, 2024 that may be closed by this pull request
Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

human facing, relatively small

lib/ldp.js Outdated Show resolved Hide resolved
lib/ldp.js Outdated Show resolved Hide resolved
test/integration/http-test.js Outdated Show resolved Hide resolved
test/integration/http-test.js Outdated Show resolved Hide resolved
test/integration/http-test.js Outdated Show resolved Hide resolved
zg009 and others added 5 commits April 1, 2024 11:01
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
wording

Co-authored-by: Ted Thibodeau Jr <[email protected]>
wording

Co-authored-by: Ted Thibodeau Jr <[email protected]>
wording

Co-authored-by: Ted Thibodeau Jr <[email protected]>
@zg009
Copy link
Contributor Author

zg009 commented Apr 1, 2024

@TallTed If it looks good to you, can you go ahead and approve the changes and I'll merge

Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK by me. Note that I code primarily in English, and cannot provide much if any useful input on the JavaScript or other code here.

@zg009
Copy link
Contributor Author

zg009 commented Apr 1, 2024

@TallTed That is OK. It is nice to have more input regarding clarity, when a test or code change has to be revisited later.

@bourgeoa bourgeoa merged commit 0b6df62 into main Apr 3, 2024
2 checks passed
@bourgeoa bourgeoa deleted the fix/issue#1692 branch April 3, 2024 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mishandling of auxiliary resource uris
3 participants